refine fix for deadlock by using non-blocking f_recv #125

samoconnor · 2018-03-04T11:26:24Z

No description provided.

samoconnor · 2018-03-04T11:28:00Z

src/ssl.jl

@@ -113,24 +111,23 @@ end

 function f_send(c_ctx, c_msg, sz)
    jl_ctx = unsafe_pointer_to_objref(c_ctx)
-    jl_msg = unsafe_wrap(Array, c_msg, sz)
-    return Cint(write(jl_ctx.bio, jl_msg))
+    return Cint(unsafe_write(jl_ctx, c_msg, sz))


Avoid allocating wrapper Array.
Just dump the bytes directly into the LibuvStream send buffer.

samoconnor · 2018-03-04T11:28:58Z

src/ssl.jl

-        n = readbytes!(jl_ctx.bio, jl_msg, sz)
+    n = nb_available(jl_ctx)
+    if n == 0
+        return Cint(MBEDTLS_ERR_SSL_WANT_READ)


If there is nothing in the LibuvStream receive buffer, tell MbedTLS that it needs to call f_recv again later.

samoconnor · 2018-03-04T11:29:38Z

src/ssl.jl

    end
+    n = min(sz, n)
+    unsafe_read(jl_ctx, c_msg, n)


avoid blocking by not trying to read more than is available in the LibuvStream receive buffer.

samoconnor · 2018-03-04T11:31:19Z

src/ssl.jl

+        if n != MBEDTLS_ERR_SSL_WANT_READ
+            mbed_err(n)
+        end
+        Base.wait_readnb(ctx.bio, 1)


If mbedtls_ssl_handshake wants more data, wait for at least one byte to be available in the LibuvStream receive buffer before trying again.

I think eof is commonly the more general thing to wait for, but are basically equivalent

palm--->face!!

samoconnor · 2018-03-04T11:32:40Z

src/ssl.jl

-        if n != MBEDTLS_ERR_SSL_WANT_READ
-            n < 0 && mbed_err(n)
+        elseif n == MBEDTLS_ERR_SSL_WANT_READ
+            Base.wait_readnb(ctx.bio, 1)


If mbedtls_ssl_read wants more data, wait for at least one byte to be available in the LibuvStream receive buffer before trying again.

vtjnash · 2018-03-04T20:07:53Z

This is looking really good! I hope it works :)

samoconnor · 2018-03-04T20:19:26Z

I hope it works too :)
But I'm left wondering why we are using MbedTLS?
It feels like it is intended for the embedded world. All the stuff I see on the forums is people working on tiny ARM systems.
Given that Julia's audience is (currently) number crunching bofins with i7 laptops and compute clusters, why are we not using the platform OpenSSL, Security.SecureTransport, or whatever the Windows equivalent is? or s2n?

I'm not arguing that we should change, I'd just like to understand why it is the way it is?

quinnj · 2018-03-04T20:23:41Z

AFAIU, OpenSSL doesn't have the greatest licensing situation. MbedTLS is a small, simple library w/ good licensing and C api, so it's a natural fit.

I think it'd be great to have an SSL.jl package that hooked into whatever platform-provided SSL was available, but that also sounds tricky to make work in a unified interface + get enough platform coverage so that anyone could use it. MbedTLS definitely has the advantage that it can pretty much be bundled/used on any platform.

samoconnor · 2018-03-04T20:33:11Z

OpenSSL doesn't have the greatest licensing situation

Can you be more specific? As far as I can see (IANAL) it says: do whatever you like but give us credit and don't blame us if its broken.

tricky to make work

Is it not proving to be tricky to make MbedTLS work?

codecov-io · 2018-03-04T20:48:49Z

Codecov Report

Merging #125 into master will increase coverage by 0.93%.
The diff coverage is 82.6%.

@@            Coverage Diff             @@
##           master     #125      +/-   ##
==========================================
+ Coverage   65.03%   65.97%   +0.93%     
==========================================
  Files          10       10              
  Lines         429      432       +3     
==========================================
+ Hits          279      285       +6     
+ Misses        150      147       -3

Impacted Files	Coverage Δ
src/ssl.jl	`69.35% <82.6%> (+3.23%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af7ab92...961b185. Read the comment docs.

samoconnor · 2018-03-04T21:09:36Z

@malmaud, I see that this was your baby way back at the start of the commit log.
I don't intend to be critical of the effort that has been put it, or of the decisions that were made along the way. But, I find myself in a position where:

There is no longer an active owner of the code.
I'm spending time fixing the problems I find.
I have found memory corruption issues, which is worrying in a cyrpto context.
My interactions with the upstream vendor are not filling me with confidence https://esp32.com/viewtopic.php?t=1101#p4945.

I'm left with a concern about the implied responsibility I'm taking on by making changes here. Users of a ctypto library have a reasonable expectation that it will keep their sensitive information secret. But I don't feel like I have enough experience with crypto to feel confident that the code will meet that expectation without more thorough review.

So, I'd really like to know a bit about the history of why MbedTLS was chosen at the time so that I can evaluate the tradeoffs I find in front of me today.

…l block before mbedtls has a chance to do its thing

…explicit

malmaud · 2018-03-05T02:10:27Z

It was mostly because of the flexible licensing - mbedtls is dual-licensed under Apache and GPL. Mbedtls generally ships with Julia to power the cryptography of the package manager. Mbedtls is used by many big companies - I don't think there's any reason to believe that it is less secure than other major crypto libraries. I'm also skeptical that the Julia wrapper could introduce security issues, since it is not handling any sensitive cryptography itself, but I guess it's theoretically possible.
Of course as you've discovered, the wrapper seems to have other (non-security-related) issues, so I think it's great that you're looking into them.

nalimilan · 2018-03-05T13:11:49Z

FWIW, the OpenSSL license is currently incompatible with the GPL, but they are fixing this by relicensing under Apache. Apparently there remains only very few contributors who haven't given their agreement, so this should be ready soon.

samoconnor · 2018-03-05T13:12:17Z

Thanks for that @malmaud.

So I found this discussion JuliaLang/julia#10763 about how FFTW's GPL license forbids the OpenSSL licence's requirement for giving credit to the authors. Seems like a good reason to remove FFTW, but so be it.

Interestingly it looks like OpenSSL will be apache licensed RSN : https://www.openssl.org/blog/blog/2018/03/01/last-license/

I had a quick play with s2n yesterday, it certainly has a simple clean API and comes from a vendor with serious motivation to do security right. One nice feature of s2n is that it handles integration with the underlying BSD sockets for you, so you don't have to worry about getting the callback functions exactly right (subject of this PR).

OSX has https://developer.apple.com/documentation/security/secure_transport I assume Windows has some MS-blessed system TLS API, and linux has OpenSSL. It would be great to be able to ship stuff to clients, and when the next TLS vulnerability comes along, just be able to say to them "don't worry, Julia uses the OS TLS library and the OS vendor has already pushed out a fix".

I think it would be nice to have a JuliaTLSClient package with OS native backends and a very small API like:

tls_connect(host, port) -> TLSHandle
unsafe_read(::TLSHandle, buf, nb)  # blocking, calls Base.poll_fd() to run main event loop
unsafe_write(::TLSHandle, buf, nb) # blocking, "
wait(::TLSHandle)                  # wait for there to be bytesavailable 
bytesavailable(::TLSHandle) -> Int

samoconnor · 2018-03-05T13:57:40Z

@nalimilan you beat me to it re OpenSSL apache license :)
I think on Linux that would be moot anyway because of the GPL's ok-to-link-with-built-in-os-libs clause.

quinnj · 2018-03-05T14:18:12Z

I think having a SSL.jl package that wrapped OS-native functionality would be great; it sounds non-trivial to me to get all the platforms to conform to a single api, but I'm also very unfamiliar w/ what all those APIs look like.

I've also been keeping an eye on https://bearssl.org/, which seems nice from a small footprint/good license standpoint.

quinnj · 2018-03-05T14:19:42Z

In any case, we could open an issue on the JuliaWeb/roadmap repo to further discuss future ssl/tls options, but for now, is this PR ready to go? CI looks good (0.7 are known failures due to BinaryProvider, which should be fixed soon). I'm happy to merge and tag (I plan on doing some dev-load testing today, so I can certainly report any issues I see).

nalimilan · 2018-03-05T14:37:02Z

It seems that Swift has the same goal of implementing a high-level SSL API using the system libraries https://forums.swift.org/t/pitch-cross-platform-swift-tls-library/5610/5

tkelman · 2018-03-05T15:45:40Z

#123 (comment) should be resolved before tagging, unless the tag is from a branch that includes this but not the BinDeps change.

All the descriptive comments would be much better in the code than in this PR, in terms of being able to find and use them later.

malmaud · 2018-03-05T15:54:26Z

Ya, the conversation about whether MbedTLS is the best choice for a particular purpose seems to make more sense for the repos that use MbedTLS, like Julia's libgit2 fork and HTTP.jl.

FWIW, I think a package that integrates with an OS's native TLS toolkit would be great, although I still don't think there's any reason to doubt the actual security aspects of MbedTLS.

samoconnor · 2018-03-05T22:50:15Z

I'm happy to merge and tag

I'm happy for you to merge, but I'd hold of on the tag until after you've done your "dev-load testing".

samoconnor · 2018-03-06T00:55:42Z

I still don't think there's any reason to doubt the actual security aspects of MbedTLS

Aside from the general degree to which one should doubt the security of everything, I agree.

However, it still seems preferable to ask a user to trust the OS vendor that they have already decided to trust, rather than ask them to trust that the Julia guys have no doubts about MbedTLS. If I take my engineer hat off and put my manager hat on, I'm going to be thinking "My dev want's to deploy a module using this Julia thing that the data guys are all raving about. But what dependencies does it pull in? Do any of them have a privacy compliance risk? Are we going to have to get experts in to do SOUP reviews?". As a dev I wan't to be able to say "don't worry, all the comms goes through the same crypto we use for everything else, it's already been certified".

samoconnor · 2018-03-06T00:57:53Z

Re: ... a package that integrates with an OS's native TLS toolkit ...

I don't know if I'll have time to work on this any time soon, but I though it was worth making some notes given that it's been on my mind. I've sketched out some ideas here:
https://github.com/samoconnor/TLSClient.jl/blob/master/README.md

Comments are welcome.
@quinnj, @malmaud, @vtjnash, @nalimilan, @tkelman, @yuyichao, @Keno

tkelman · 2018-03-06T01:15:04Z

I'll just say that for historical context, there have been somewhere between 2-5 distinct attempts at writing OpenSSL bindings for Julia in the past. I'm not going to try to comprehensively list them, but you can look around and find them - to my knowledge none have seen any development at any time within the last year or two, or longer. There was GnuTLS.jl which was actively used for a while (mostly for license compatibility reasons) but it had its set of problems and fell out of favor rapidly as soon as Jon wrote this package and got it up and functional.

When you work with OpenSSL, you also need to worry about how its API has changed over time, and the widely varying versions out there in the wild on different ages of Linux distro. The forks like LibreSSL and BoringSSL are also viable options. I don't know whether anyone has tried to write Julia bindings for WinHTTP or SecureTransport.

https://blog.regehr.org/archives/1261 is also an interesting anecdote, but from a few years ago.

samoconnor · 2018-03-06T01:44:39Z

Good points Tony, let's continue over here: JuliaWeb/TLSClient.jl#1 (comment)

samoconnor · 2018-03-08T00:21:25Z

@quinnj are we ready to tag this yet?

JuliaLang/MbedTLS.jl#125

refine fix for deadlock by using non-blocking f_recv

87744f1

samoconnor commented Mar 4, 2018

View reviewed changes

samoconnor mentioned this pull request Mar 4, 2018

Try to fix deadlock #124

Merged

use eof instead of wait_readnb

5afe034

samoconnor added 2 commits March 5, 2018 08:13

dont call eof until after calling mbedtls ccalls. otherwise eof() wil…

6a0391a

…l block before mbedtls has a chance to do its thing

define wait(ctx) = eof(ctx.bio) to make intention to block/wait more …

961b185

…explicit

samoconnor mentioned this pull request Mar 6, 2018

Discussion JuliaWeb/TLSClient.jl#1

Open

quinnj merged commit e6b366c into JuliaLang:master Mar 6, 2018

samoconnor added a commit to JuliaWeb/HTTP.jl that referenced this pull request Mar 13, 2018

Update REQUIRE for MbedTLS.jl v0.5.7 deadlock fix

aacb40c

JuliaLang/MbedTLS.jl#125

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refine fix for deadlock by using non-blocking f_recv #125

refine fix for deadlock by using non-blocking f_recv #125

samoconnor commented Mar 4, 2018

samoconnor Mar 4, 2018

samoconnor Mar 4, 2018

samoconnor Mar 4, 2018

samoconnor Mar 4, 2018

vtjnash Mar 4, 2018

samoconnor Mar 4, 2018

samoconnor Mar 4, 2018

vtjnash commented Mar 4, 2018

samoconnor commented Mar 4, 2018 •

edited

Loading

quinnj commented Mar 4, 2018

samoconnor commented Mar 4, 2018

codecov-io commented Mar 4, 2018 •

edited

Loading

samoconnor commented Mar 4, 2018

malmaud commented Mar 5, 2018

nalimilan commented Mar 5, 2018

samoconnor commented Mar 5, 2018 •

edited

Loading

samoconnor commented Mar 5, 2018

quinnj commented Mar 5, 2018

quinnj commented Mar 5, 2018

nalimilan commented Mar 5, 2018

tkelman commented Mar 5, 2018

malmaud commented Mar 5, 2018

samoconnor commented Mar 5, 2018

samoconnor commented Mar 6, 2018

samoconnor commented Mar 6, 2018

tkelman commented Mar 6, 2018

samoconnor commented Mar 6, 2018

samoconnor commented Mar 8, 2018

refine fix for deadlock by using non-blocking f_recv #125

refine fix for deadlock by using non-blocking f_recv #125

Conversation

samoconnor commented Mar 4, 2018

samoconnor Mar 4, 2018

Choose a reason for hiding this comment

samoconnor Mar 4, 2018

Choose a reason for hiding this comment

samoconnor Mar 4, 2018

Choose a reason for hiding this comment

samoconnor Mar 4, 2018

Choose a reason for hiding this comment

vtjnash Mar 4, 2018

Choose a reason for hiding this comment

samoconnor Mar 4, 2018

Choose a reason for hiding this comment

samoconnor Mar 4, 2018

Choose a reason for hiding this comment

vtjnash commented Mar 4, 2018

samoconnor commented Mar 4, 2018 • edited Loading

quinnj commented Mar 4, 2018

samoconnor commented Mar 4, 2018

codecov-io commented Mar 4, 2018 • edited Loading

Codecov Report

samoconnor commented Mar 4, 2018

malmaud commented Mar 5, 2018

nalimilan commented Mar 5, 2018

samoconnor commented Mar 5, 2018 • edited Loading

samoconnor commented Mar 5, 2018

quinnj commented Mar 5, 2018

quinnj commented Mar 5, 2018

nalimilan commented Mar 5, 2018

tkelman commented Mar 5, 2018

malmaud commented Mar 5, 2018

samoconnor commented Mar 5, 2018

samoconnor commented Mar 6, 2018

samoconnor commented Mar 6, 2018

tkelman commented Mar 6, 2018

samoconnor commented Mar 6, 2018

samoconnor commented Mar 8, 2018

samoconnor commented Mar 4, 2018 •

edited

Loading

codecov-io commented Mar 4, 2018 •

edited

Loading

samoconnor commented Mar 5, 2018 •

edited

Loading